Add Go 1.20 errors.Join support #118
Conversation
There was a problem hiding this comment.
May I suggest:
- move the new join.go to its sub-package
joinimmediately in the 1st commit. - ensure the
joinErrortype also implementsSafeFormatError. Then express Error/Format using that, using the model already set by other types. - in the 2nd commit, make the top-level join
Joinpoint to a wrapper in packageerrutilthat follows the example ofWrapand also automatically adds a call toWithStack.
Reviewable status: 0 of 18 files reviewed, all discussions resolved
There was a problem hiding this comment.
Also, for your consideration:
What about not adding a joinError type altogether, and instead construct an opaqueMultiErrorLeaf with all the formatting done upfront (in errutil)?
(This would need to use a redact.RedactableString)
Reviewable status: 0 of 18 files reviewed, all discussions resolved
There was a problem hiding this comment.
(maybe my last consideration is not so great - the opaque type's support for redactable strings is janky)
If you do not do that, consider adding a decoder for the new joinError type.
Reviewable status: 0 of 18 files reviewed, all discussions resolved
dhartunian
force-pushed
the
go-1.20-join
branch
from
6e19cb9 to
85063cc
Compare
|
@knz PTAL
|
|
CI is trying to tell you something. I'd say let's merge #115 then you can rebase this which will make the review easier. |
dhartunian
force-pushed
the
go-1.20-join
branch
2 times, most recently
from
9b2dd64 to
5a7989c
Compare
lol fixed 😄 |
There was a problem hiding this comment.
Reviewed 17 of 18 files at r5, 8 of 8 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dhartunian)
go.mod line 3 at r6 (raw file):
module github.com/cockroachdb/errors go 1.18
I'm not sure you wanted to change this?
join/join.go line 76 at r6 (raw file):
func (e *joinError) SafeFormatError(p errbase.Printer) error { p.Print(e.Error())
Please do it the other way around: put the logic in here, then do Error() string { return redact.StringWithoutMarkers(e) }
Otherwise you'd be throwing redactability out of the window.
(you can see the effects of this in the format tests)
This commit introduces a drop-in replacement of the go errors lib `Join` method which constructs a simple multi-cause error object. Some simple unit tests are added and `Join` wrappers are also integrated into the datadriven formatting test. Our existing format code for multi-cause errors is compatible with this new type which allows us to remove the custom formatter from the implementation. The public Join API contains wrappers that automatically attach stacktraces to the join errors. Signed-off-by: Steve Coffman <[email protected]>
dhartunian
force-pushed
the
go-1.20-join
branch
from
5a7989c to
89c9f9e
Compare
There was a problem hiding this comment.
Reviewable status: 13 of 25 files reviewed, 2 unresolved discussions (waiting on @knz)
go.mod line 3 at r6 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
I'm not sure you wanted to change this?
whoops. done.
join/join.go line 76 at r6 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
Please do it the other way around: put the logic in here, then do
Error() string { return redact.StringWithoutMarkers(e) }Otherwise you'd be throwing redactability out of the window.
(you can see the effects of this in the format tests)
done.
There was a problem hiding this comment.
💯
Reviewed 12 of 12 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dhartunian)
This PR contains the
Joinimplementation from #106 and addssome adjustments to fit the rest of the library's style.
We move the newly added
Joinimplementation that mirrorsthe go 1.20 stdlib function into a separate package like the rest of
our custom error types.
Some simple unit tests are added and
Joinwrappers are alsointegrated into the datadriven formatting test. Our existing format
code for multi-cause errors is compatible with this new type which
allows us to remove the custom formatter from the implementation.
Please review last 2 commits. First one is #115.
This change is